-
Notifications
You must be signed in to change notification settings - Fork 3.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add a New ConfigurableX509TrustManager Class #6805
Conversation
import javax.net.ssl.TrustManagerFactory; | ||
import javax.net.ssl.X509ExtendedTrustManager; | ||
|
||
public class ConfigurableX509TrustManager extends X509ExtendedTrustManager { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Javadoc comments for public class?
- Can it be marked
final
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would expect some other users to further extend this class. In that case, it is better to not make it final
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When would a user extend the class instead of delegating to the class? Since all the protected+public methods are public in X509ExtendedTrustManager, I can't figure out any purpose to extending this class. As a general rule it is much better to mark things final
.
netty/src/main/java/io/grpc/netty/ConfigurableX509TrustManager.java
Outdated
Show resolved
Hide resolved
I can see configurable trust manager being useful but this PR has no gRPC specific code nor netty specific code - but just an implementation of |
af2d9ca
to
39a2363
Compare
@sanjaypujare |
There is nothing netty specific in this PR either. I think the best place might be an example in grpc where we include these classes with a concrete example of configurable trust manager. |
I looked at the Netty link you posted and it does have util classes so this could fit right in there (and will have more audience than just gRPC). For grpc examples you will need to create a proper example which includes client and server. The added advantage is that you can then show I talked to @ejona86 offline and he'll add his comments too. |
@ejona86 I couldn't add you as the reviewer but feel free to comment on this PR. |
* level of peer checking mechanisms, as well as some customized check. It could also be used to | ||
* reload trust certificate bundle client/server uses. | ||
*/ | ||
public class ConfigurableX509TrustManager extends X509ExtendedTrustManager { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All new APIs should be marked as @ExperimentalApi
. You can add the annotation to each of the public classes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the suggestions! Shall I also create an issue to keep track of it, like this one: #1710?
* 2. provide custom peer verification check by inheriting |verifyPeerCertificate| | ||
* 3. change the trust CA certificate bundle by inheriting |getTrustedCerts| | ||
*/ | ||
public abstract class TlsOptions { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So this is expected to be extended by users? It seems it would be best to remove private VerificationAuthType verificationType;
and just have users implement the getVerificationAuthType()
method directly.
Since this is intended to be extended with custom behavior, "options" doesn't really fit as a name.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, I'd like to discuss a bit more about this here. Making it an interface seems a reasonable common practice, but the implication becomes slightly different. Before, I want users to set a VerificationAuthType
at the construction time, and this can be expressed by making it a construction function of an abstract class; If using interface, users may need to read the comments to know they need to set VerificationAuthType
. The method might be more like setVerificationAuthType
instead of getVerificationAuthType
if using interface. May I know your opinions?
Also, does TlsConfig
sound like a better name for you?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was not suggesting to make it an interface; keep it as an abstract class. We use abstract classes so we can add methods to them later.
No, even if the user is implementing it, getVerificationAuthType()
is a proper name for it. I don't think that would cause a problem for Java programmers, especially those that would be implementing this.
The other option is to guarantee that the verification type does not change over time and make getVerificationAuthType()
final. The problem now is that it can be overridden but it is also required for the constructor.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SG. Updated accordingly.
Regarding the name of this class, would TlsConfig
sound better?
import javax.net.ssl.TrustManagerFactory; | ||
import javax.net.ssl.X509ExtendedTrustManager; | ||
|
||
public class ConfigurableX509TrustManager extends X509ExtendedTrustManager { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When would a user extend the class instead of delegating to the class? Since all the protected+public methods are public in X509ExtendedTrustManager, I can't figure out any purpose to extending this class. As a general rule it is much better to mark things final
.
netty/src/main/java/io/grpc/netty/ConfigurableX509TrustManager.java
Outdated
Show resolved
Hide resolved
netty/src/main/java/io/grpc/netty/ConfigurableX509TrustManager.java
Outdated
Show resolved
Hide resolved
|
||
@Override | ||
public X509Certificate[] getAcceptedIssuers() { | ||
return new X509Certificate[0]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like this could be implemented, but using KeyStore.aliases()
and KeyStore.getCertificate(alias)
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Implementing it will cause one test to fail, and I think the cause is probably https://github.com/netty/netty/blob/netty-4.1.48.Final/handler/src/main/java/io/netty/handler/ssl/ReferenceCountedOpenSslServerContext.java#L153-L164. It looks like if setting this, Netty internally will use it to do some additional checks, which is not the behavior I want. I might want checkClientTrusted
and checkServerTrusted
to cover all the trust behaviors we might have during the authentication.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm... That's sort of worrisome. Go ahead and put a comment here saying why it is empty.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually... this looks like it is only for certificate chain verification. What does it break?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've summarized my findings in this Gist: https://gist.github.com/ZhenLian/50c73f146493ac2fe9746d196082679b.
If I changed getAcceptedIssuers
to the one named change.java
in my Gist, one of the tests I wrote would fail, with the error message named failure.log
.
That is actually an expected behavior under my current implementation. The failing test is basically to test under the scenario that server chooses to SkipAllVerification
, and client sends a bad certificate, in mTLS. This should be OK because server chooses to skip all verification(in real situations, we want application to provide additional custom checks, but since this is test, we just test the simple behavior). But this test won't pass if we have additional check in getAcceptedIssuers
.
netty/src/main/java/io/grpc/netty/ConfigurableX509TrustManager.java
Outdated
Show resolved
Hide resolved
String algorithm = authType == VerificationAuthType.CertificateAndHostNameVerification | ||
? "HTTPS" : ""; | ||
SSLParameters sslParams = sslEngine.getSSLParameters(); | ||
sslParams.setEndpointIdentificationAlgorithm(algorithm); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is surprising this is being set, since these variables are normally set elsewhere are read within the trust manager as configuration. This looks suspicious. It will also permanently change the parameters, which seems concerning. Have you verified that it is safe to disable endpoint identification here and it not impact any other code?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, thanks for the reminding! I have tested it out locally and it seems good. I also added some integration tests(basicClientSideIntegrationTest
and basicServerSideIntegrationTest
), and it looks like this EndpointIdentificationAlgorithm
behaves as expected, without impacting other parts of the code. Please let me know if you find anything suspicious, thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"I tested it once" isn't what I really meant by verify. But since this on the engine, it is probably okay.
netty/src/main/java/io/grpc/netty/ConfigurableX509TrustManager.java
Outdated
Show resolved
Hide resolved
88eba1a
to
d45e51a
Compare
d45e51a
to
624edfd
Compare
@ejona86 |
netty/src/main/java/io/grpc/netty/ConfigurableX509TrustManager.java
Outdated
Show resolved
Hide resolved
|
||
@Override | ||
public X509Certificate[] getAcceptedIssuers() { | ||
return new X509Certificate[0]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm... That's sort of worrisome. Go ahead and put a comment here saying why it is empty.
82781db
to
89b5c4f
Compare
I've updated the tests and now the coverage issue is gone. I've also added some more comments since your last review @ejona86 Could you please take a look when you have time please? Thanks! |
@ejona86 Friendly ping :-) |
Is #8175 intended to replace this PR? |
This PR contains the following three classes:
TlsOptions.java: an option set by users to specify some advanced features when performing TLS handshake, both on client side and on server side. These features include:
ConfigurableX509TrustManager.java: a class that extends X509ExtendedTrustManager and takes TlsOptions as parameter. Users need to feed ConfigurableX509TrustManager when creating GrpcSslContexts, in order to use the advanced features.
AdvancedTlsTest.java: integration tests.